-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ECO-5170] Improve API and internals for presence data #189
Conversation
These are not like the `Message.asQueryItems` method, since, unlike that one, their result is not intended to be used as the query params of an HTTP request.
Makes it clearer that we don’t care about data in these calls. I should have done this in 4ee16bd.
This inconsistency with the `data` property was a mistake in 20e7f5f.
There was one place where we throw and one where we fail silently (even though the surrounding code throws for a bunch of other reasons of seemingly similar severity). So throw consistently.
Public API improvements: - You can now pass any JSON value as presence data (previously, the top-level value had to be an object, and you could not pass arrays or nested objects). This brings us in line with CHA-PR2a. - Do not expose the `userCustomData` property of the presence data object in the public API; it’s an implementation detail. - Conform to the `ExpressibleBy*Literal` protocols, making it easier to create a JSON value. I have deliberately chosen to make the data-arg variants of the presence operation methods take a non-optional PresenceData. This is to minimise confusion between the absence of presence data and a presence data with JSON value `null`. This is how I wrote this API in 20e7f5f, but I didn’t restore it properly in 4ee16bd. The JSONValue type introduced here is based on the example given in [1]. Internals improvements: - Fix the presence object that gets passed to ably-cocoa when the user specifies no presence data; we were previously passing an empty string as the presence data in this case. (See below for where I got the “correct” behaviour from.) - Simplify the way in which we decode the presence data received from ably-cocoa (i.e. don’t do a round-trip of JSON serialization and deserialization); this comes at the cost of not getting a little bit for free from Swift’s serialization mechanism, but I think it’s worth it The behaviour of how to map the chat presence data public API to the object exchanged with the core SDK is not currently fully specified. So, the behaviour that I’ve implemented here is based on the behaviour of the JS Chat SDK at 69ea478. I’ve created spec issue [2] in order to specify this stuff properly, but I’m in a slight rush to get this public API fixed before we release our first beta, so I’ll address this later. Resolves #178. [1] https://www.douggregor.net/posts/swift-for-cxx-practitioners-literals/ [2] ably/specification#256
WalkthroughThe pull request introduces significant changes to the handling of presence data in the AblyChat application. Key modifications include the simplification of data structures used for entering presence in chat rooms, refactoring of mock presence methods for flexibility, and the introduction of a new type-safe enumeration for JSON values. Additionally, methods across various classes have been updated to enforce stricter data handling and improve error logging. Overall, these changes streamline the presence management system and enhance the clarity of data interactions. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
Sources/AblyChat/Presence.swift (1)
74-74
: Clarify the Meaning ofnil
vs.JSONValue.null
in Presence DataThe comment clarifies that
nil
indicates the absence of presence data, whereas aJSONValue
of case.null
represents an explicitnull
value. This distinction is important for correctly interpreting presence data.Consider enhancing the documentation to emphasize this distinction, possibly by providing examples, to aid developers in understanding the difference.
Tests/AblyChatTests/PresenceDataDTOTests.swift (1)
4-41
: Consider Adding Additional Test Cases for Complex Data StructuresWhile the existing tests cover basic scenarios, consider adding test cases with more complex
PresenceData
, such as nested objects or arrays, to ensure thatPresenceDataDTO
handles all possible JSON structures correctly.For example, test with:
- Nested JSON objects
- Arrays of values
- Mixed data types
This will enhance test coverage and ensure robustness.
Example/AblyChatExample/Mocks/MockClients.swift (1)
346-354
: Consider removing redundant default parameterThe private
leave(dataForEvent:)
method includes a default parameter, which is redundant since the overloaded methods already handle both cases.- func leave(dataForEvent: PresenceData? = nil) async throws { + private func leave(dataForEvent: PresenceData?) async throws {Tests/AblyChatTests/IntegrationTests.swift (1)
208-241
: LGTM: Thorough presence data testing with opportunity for refactoringThe tests comprehensively verify presence operations with data. However, there's repetition in the test pattern that could be extracted into a helper method.
Consider creating a helper method to reduce repetition:
private func verifyPresenceOperation( operation: (PresenceData) async throws -> Void, expectedAction: PresenceEventType ) async throws { let data: [String: String] = ["randomData": "randomValue"] try await operation(data) let event = try await rxPresenceSubscription.first { _ in true } #expect(event.action == expectedAction) #expect(event.data == data) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
Example/AblyChatExample/ContentView.swift
(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift
(3 hunks)Sources/AblyChat/DefaultPresence.swift
(10 hunks)Sources/AblyChat/DefaultRoomReactions.swift
(1 hunks)Sources/AblyChat/JSONValue.swift
(1 hunks)Sources/AblyChat/Presence.swift
(3 hunks)Sources/AblyChat/PresenceDataDTO.swift
(1 hunks)Sources/AblyChat/RoomReactions.swift
(1 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift
(1 hunks)Tests/AblyChatTests/IntegrationTests.swift
(3 hunks)Tests/AblyChatTests/JSONValueTests.swift
(1 hunks)Tests/AblyChatTests/PresenceDataDTOTests.swift
(1 hunks)
🔇 Additional comments (24)
Sources/AblyChat/RoomReactions.swift (1)
32-35
: LGTM! Clear documentation improvement
The renamed method asJSONObject()
better reflects its purpose, and the added documentation clearly explains its use case with ably-cocoa's publish operation.
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
42-42
: LGTM! Test updated to match API changes
The test assertion has been correctly updated to use the renamed asJSONObject()
method, maintaining test coverage while reflecting the API improvements.
Sources/AblyChat/DefaultRoomReactions.swift (2)
27-27
: LGTM! Consistent with API changes
The send method correctly uses the renamed asJSONObject()
method, maintaining consistency with the API improvements.
Line range hint 3-5
: Verify if @mainactor is the best solution
While @mainactor fixes the task isolation error, the TODO comment suggests this might be temporary. Consider investigating alternative solutions as part of issue #83.
Tests/AblyChatTests/JSONValueTests.swift (1)
1-145
: Unit tests are comprehensive and validate all JSONValue cases
The test suite thoroughly covers the conversion between JSONValue
and Ably Cocoa presence data, including end-to-end serialization and deserialization. This ensures reliability and correctness of the JSON handling.
Sources/AblyChat/Presence.swift (3)
3-3
: Simplify PresenceData with Typealias
The PresenceData
typealias has been updated to JSONValue
, simplifying the representation of presence data throughout the codebase.
65-65
: Adjust Initializer to Accept Optional PresenceData
The initializer for PresenceMember
now accepts an optional PresenceData?
for the data
parameter. This allows for representing the absence of presence data with nil
, distinguishing it from a JSONValue.null
.
9-13
: Update Method Signatures to Require Non-Optional PresenceData
The enter
, update
, and leave
methods in the Presence
protocol now require a non-optional PresenceData
parameter. This enforces that presence data must be provided when these methods are called, enhancing data integrity.
Please verify that all calls to these methods in the codebase provide a non-optional PresenceData
. Run the following script to identify any calls passing nil
:
✅ Verification successful
Based on the search results, I can now generate the final response:
Non-Optional PresenceData Parameter Usage is Consistent
The verification shows that all calls to enter
, update
, and leave
methods across the codebase consistently provide non-optional PresenceData:
- Test files use dictionary literals:
["randomData": "randomValue"]
- Example app uses:
["status": "📱 Online"]
- Implementation in
DefaultPresence.swift
uses transformed data:JSONValue.object(dto.toJSONObjectValue).toAblyCocoaPresenceData
No instances of nil
usage were found, confirming that the non-optional parameter requirement is being followed correctly throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `enter`, `update`, and `leave` methods passing `nil` for the `data` parameter.
rg '\.(enter|update|leave)\(data:\s*nil\)' -t swift
Length of output: 53
Script:
#!/bin/bash
# Let's search for all calls to these methods to understand their usage patterns
rg '\.(enter|update|leave)\((data:)?[^)]' -t swift -A 1
Length of output: 2278
Script:
#!/bin/bash
# Let's also check the protocol implementations to understand how these methods are used
ast-grep --pattern 'func $_(data: PresenceData) async throws'
Length of output: 63
Script:
#!/bin/bash
# Let's check if there are any overloads without the data parameter
ast-grep --pattern 'func $_(data: $_) async throws'
Length of output: 53
Sources/AblyChat/DefaultPresence.swift (5)
95-101
: Refactor enter
Method for Clarity
The enter
method has been refactored to include two overloads:
enter(data: PresenceData)
which accepts presence data.enter()
which callsenter(optionalData: nil)
internally.
This separation enhances clarity and enforces explicit data handling.
Line range hint 104-116
: Private Method enter(optionalData:)
Handles Optional Data
The private method enter(optionalData:)
consolidates the logic for entering presence with optional data. It constructs a PresenceDataDTO
with userCustomData: data
, ensuring consistent data handling.
129-136
: Apply Consistent Refactoring to update
Method
Similar to enter
, the update
method has been refactored with two overloads and a private update(optionalData:)
method. This promotes consistency and clarity in presence data operations.
163-170
: Implement Consistent Pattern for leave
Method
The leave
method now follows the same pattern as enter
and update
, with overloads and a private leave(optionalData:)
method. This consistency improves maintainability.
233-246
: Improve Error Handling in decodePresenceDataDTO
Method
The decodePresenceDataDTO
method now properly handles cases where ablyCocoaPresenceData
is nil
by throwing a descriptive error. This ensures that unexpected data formats are correctly managed.
Sources/AblyChat/PresenceDataDTO.swift (3)
1-34
: Introduce PresenceDataDTO
for Structured Presence Data
The new PresenceDataDTO
struct provides a clear and structured way to handle presence data, encapsulating the userCustomData
. It includes methods for initializing from a JSONValue
and converting back to a JSON object, which enhances serialization and deserialization processes.
17-23
: Ensure Robust Initialization from JSON Value
The initializer init(jsonValue:)
checks that the provided jsonValue
is an object and correctly extracts userCustomData
. This validation prevents runtime errors due to incorrect data types.
25-33
: Accurately Convert DTO to JSON Object Value
The toJSONObjectValue
computed property efficiently constructs a JSON object from the PresenceDataDTO
, including userCustomData
only when it is not nil
. This ensures that unnecessary keys are not included in the serialized data.
Tests/AblyChatTests/PresenceDataDTOTests.swift (3)
7-17
: Validate Initialization from JSON Value with Comprehensive Test Cases
The initWithJSONValue
test function covers multiple scenarios, ensuring that PresenceDataDTO
initializes correctly from various JSON inputs, including cases with missing keys and null
values.
19-23
: Test Failure when Initializing with Non-Object JSON Value
The initWithJSONValue_failsIfNotObject
test confirms that an error is thrown when attempting to initialize PresenceDataDTO
with a non-object JSON value. This ensures robust error handling in the presence data deserialization process.
27-40
: Ensure Correct Serialization in toJSONObject
Tests
The toJSONObject
test function verifies that PresenceDataDTO
serializes to a JSON object accurately, covering scenarios where userCustomData
is nil
, a non-null value, or explicitly .null
.
Example/AblyChatExample/Mocks/MockClients.swift (3)
280-280
: LGTM: Simplified presence data handling
The change to return nil
for presence data in mock implementations aligns with the PR objectives to streamline the API.
Also applies to: 292-292
304-312
: LGTM: Well-structured presence enter methods
The implementation provides a clean API with:
- Overloaded methods for flexibility
- Private shared implementation for DRY principle
- Thread-safety maintained within the actor
325-333
: LGTM: Consistent update method implementation
The update methods follow the same pattern as enter methods, maintaining API consistency.
Tests/AblyChatTests/IntegrationTests.swift (1)
179-179
: LGTM: Simplified presence method calls
The removal of explicit nil parameters makes the API more concise while maintaining the same functionality.
Also applies to: 191-191
Example/AblyChatExample/ContentView.swift (1)
205-205
: LGTM: Simplified presence data handling in UI
The changes streamline presence data handling by:
- Using direct dictionary format for status
- Accessing data through type-safe accessors
Also applies to: 211-211
✅ Verification successful
Presence data handling is consistent across the codebase
The verification confirms that:
- The presence data format using dictionary literals is consistently used across the codebase
- The access pattern using
objectValue?["status"]?.stringValue
is correctly implemented for reading presence data - Test cases demonstrate similar patterns for presence data handling with
enter
andupdate
operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent presence data access pattern across the codebase
rg -A 2 'objectValue\?\["status"\]'
Length of output: 493
Script:
#!/bin/bash
# Search for presence enter/update patterns to verify consistency
rg -A 2 'presence\.(enter|update).*data:'
Length of output: 2453
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Public API improvements
userCustomData
property of the presence data object in the public API; it’s an implementation detail.ExpressibleBy*Literal
protocols, making it easier to create a JSON value.I have deliberately chosen to make the data-arg variants of the presence operation methods take a non-optional
PresenceData
. This is to minimise confusion between the absence of presence data and a presence data with JSON valuenull
. This is how I wrote this API in 20e7f5f, but I didn’t restore it properly in 4ee16bd.The
JSONValue
type introduced here is based on the example given here.Internals improvements
The behaviour of how to map the chat presence data public API to the object exchanged with the core SDK is not currently fully specified. So, the behaviour that I’ve implemented here is based on the behaviour of the JS Chat SDK at
69ea478
. I’ve created spec issue ably/specification#256 in order to specify this stuff properly, but I’m in a slight rush to get this public API fixed before we release our first beta, so I’ll address this later.Resolves #178.
Summary by CodeRabbit
New Features
JSONValue
type-safe enumeration for representing various JSON data types.PresenceDataDTO
structure for handling presence data in a JSON format.Improvements
Tests
JSONValue
andPresenceDataDTO
to ensure functionality and robustness.